🎨 calculator 코드 구현 not w/ testcode by hansolnoh95 · Pull Request #5 · Roadfinder/java-calculator

hansolnoh95 · 2021-04-22T08:52:09Z

No description provided.

KimGyeong

코멘트 확인해주세요.

KimGyeong · 2021-04-22T08:53:42Z

src/main/java/Calculator.java

+    static private final String ADD_OPERATION = "+";
+    static private final String SUBTRACT_OPERATION = "-";
+    static private final String MULTIPLY_OPERATION = "*";
+    static private final String DIVIDE_OPERATION = "/";
+    static private final int NUMBER_INDEX = 0;
+    static private final int OPERATION_INDEX = 1;


Suggested change

static private final String ADD_OPERATION = "+";

static private final String SUBTRACT_OPERATION = "-";

static private final String MULTIPLY_OPERATION = "*";

static private final String DIVIDE_OPERATION = "/";

static private final int NUMBER_INDEX = 0;

static private final int OPERATION_INDEX = 1;

private static final String ADD_OPERATION = "+";

private static final String SUBTRACT_OPERATION = "-";

private static final String MULTIPLY_OPERATION = "*";

private static final String DIVIDE_OPERATION = "/";

private static final int NUMBER_INDEX = 0;

private static final int OPERATION_INDEX = 1;

private는 접근제어자입니다. 모든 선언에서 접근제어자는 맨 처음으로 나와주어야 합니다.

KimGyeong · 2021-04-22T08:56:15Z

src/main/java/Calculator.java

+    List<String> numberList = new ArrayList<>();
+    List<String> operationList = new ArrayList<>();
+    int result = 0;


Calculator의 필드변수들입니다. 접근제어자를 작성해주지 않으면 default라는 접근제어자가 기본값으로 적용됩니다. default는 같은 패키지 내의 다른 클래스가 사용할 수 있습니다. 다른 클래스에서 접근하지 못하게 private으로 변경해주세요.

KimGyeong · 2021-04-22T09:00:05Z

src/main/java/Calculator.java

+        validation(line);
+        addNumberList(line);
+        addOperationList(line);


Calculator클래스의 메소드 중 다른 클래스에서 사용하는 메소드는 execute() 단 하나입니다.
그래서 이 메소드들의 접근제어자를 private으로 변경하는게 좋을 수 있습니다.
단, private 메소드들은 테스트를 하기 어려워집니다. 테스트 클래스에서 바로 호출할 수 없기 때문이죠.
execute()와 같은 메소드가 존재할 경우에 발생하는 문제는 이렇습니다. 이 메소드가 수행하는 로직을 main()에서 진행하는 것을 권장합니다.

Calculator 내에 main()을 만들라는 말인가여?

main 메소드의 길이를 신경쓰지 않으셔도 된다는 말이에요. 제 개인적인 의견이지만 main에서 이 프로그램이 어떻게 흘러가는지를 알 수 있게끔 구현해도 괜찮다고 생각합니다.
추가적으로 공식에 대한 예외처리를 과연 Calculator가 해야하는 지에 대한 의문도 드네요.

KimGyeong · 2021-04-22T09:00:53Z

src/main/java/Input.java

+import java.util.Scanner;
+
+public class Input {
+    public static final String BLANK = " ";


Suggested change

public static final String BLANK = " ";

private static final String BLANK = " ";

접근제어자를 변경해주세요.

KimGyeong · 2021-04-22T09:01:39Z

src/main/java/Main.java

+        }
+    }
+
+    public static String[] getStream() {


메소드명의 의미 전달이 불확실합니다. 메소드명을 변경해주세요.

KimGyeong · 2021-04-22T09:02:48Z

src/main/java/Output.java

+        System.out.println(result);
+    }
+    public static void printError(Exception e) {
+        System.out.println("출력 에러");


메소드명으로 에러를 출력함을 알 수 있는데, 어떤 에러인지 알 수 없습니다. 메소드명을 변경해주세요.

KimGyeong · 2021-04-22T09:05:57Z

src/main/java/Calculator.java

+    }
+
+    public void validation(String line[]) {
+        if (line.length%2 == 0) throw new RuntimeException("올바른 식을 완성해주세요.");


조건문은 어떤 것을 의미할까요? 이 메소드만 보았을 때에 입력받은 line의 길이가 짝수이면 올바른 식이 아니라는 말로 받아들일 수 있는데, line은 어떤 것을 의미하는지 모를 수 있겠네요.

KimGyeong · 2021-04-22T09:06:25Z

src/main/java/Calculator.java

+        return result;
+    }
+
+    public void validation(String line[]) {


Suggested change

public void validation(String line[]) {

public void validation(String[] line) {

Array의 선언은 일반적으로 타입뒤에 대괄호를 붙입니다. 아래도 동일하게 변경해주세요.

KimGyeong · 2021-04-22T09:07:16Z

src/main/java/Input.java

+public class Input {
+    public static final String BLANK = " ";
+
+    public static String getInput() {


이처럼 'get'으로 시작하는 메소드는 클래스의 필드값을 반환해주는 로직을 수행하는 것을 권장합니다. 메소드명을 변경해주세요. 어떤 것을 입력받는지에 대해 알려주면 좋을 것 같습니다.

KimGyeong

제 코멘트가 무조건 맞지 않아요. 코멘트에 대해 생각해보고 반박하는 마음가짐이면 좋겠습니다. 코멘트 확인해주세요.

KimGyeong · 2021-04-24T16:20:48Z

src/main/java/Calculator.java

+        validation(line);
+        addNumberList(line);
+        addOperationList(line);


main 메소드의 길이를 신경쓰지 않으셔도 된다는 말이에요. 제 개인적인 의견이지만 main에서 이 프로그램이 어떻게 흘러가는지를 알 수 있게끔 구현해도 괜찮다고 생각합니다.
추가적으로 공식에 대한 예외처리를 과연 Calculator가 해야하는 지에 대한 의문도 드네요.

KimGyeong · 2021-04-24T16:22:13Z

src/main/java/Main.java

+        }
+    }
+
+    public static String[] getFormattedInput() {


이 메소드 또한 이전의 코멘트처럼 get으로 시작하는 메소드이네요. input으로 시작하는 이름으로 지으면 헷갈리는 일이 덜 발생할 것이라고 생각해요.

KimGyeong · 2021-04-24T16:24:04Z

src/main/java/Calculator.java

+    private void validation(String[] formattedInput) {
+        if (formattedInput.length%2 == 0) throw new RuntimeException("올바른 식을 완성해주세요.");
+    }


개발자가 편하면 사용자가 불편하다.라고 생각해요. 올바른 식은 어떤 것일까요? 숫자 두 개를 연속으로 입력한 식은 올바른 식일까요? 연산자 두 개를 연속으로 입력한 식은 올바른 식일까요? 예외에 대해 조금 더 깊게 생각해보는 것도 중요하다고 생각합니다.

KimGyeong · 2021-04-24T17:10:45Z

src/main/java/Calculator.java

+    private List<String> numberList = new ArrayList<>();
+    private List<String> operationList = new ArrayList<>();
+    private int result = 0;


이렇게 인스턴스 변수를 초기화하는 것을 명시적 초기화라고 합니다. 생성자를 통해 인스턴스 변수를 초기화 할 수 있습니다. 인스턴스 변수 초기화의 방법은 4가지가 있고, 각 초기화가 실행되는 시점이 다릅니다. 이를 알고 있으면 좋을 것 같아요.

🎨 calculator 코드 구현 not w/ testcode

826b136

KimGyeong reviewed Apr 22, 2021

View reviewed changes

🎨 2차 피드백 반영 완료

79e2a8d

KimGyeong reviewed Apr 24, 2021

View reviewed changes

	public static final String BLANK = " ";
	private static final String BLANK = " ";

	public void validation(String line[]) {
	public void validation(String[] line) {

Conversation

hansolnoh95 commented Apr 22, 2021

Uh oh!

KimGyeong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KimGyeong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants